Skip to content

Conversation

@michalsn
Copy link
Member

Description
Fixes #2143 - getWhere() bug with offset.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@jim-parry jim-parry changed the title Fixes #2143 Fixes BaseBuilder getWhere() bug Sep 20, 2019
@jim-parry
Copy link
Contributor

You have added returnSQL and reset parameters to the getWhere() signature.
These don't seem related to the default values bug.
Am I missing something?

@michalsn
Copy link
Member Author

  1. returnSQL - without it this method is not testable.
  2. reset - I feel it should be consistent with the get() method - it just makes more sense to me... so I don't see a reason not to allow reset in this method too.

@jim-parry
Copy link
Contributor

@lonnieezell @MGatner Makes sense. Are you good with this?

@MGatner
Copy link
Member

MGatner commented Sep 24, 2019

It looks good to me, really nice tests. It's unfortunate how all the database & builder methods keep growing in number of parameters but I think @michalsn is right that the current implementation needs those. Maybe down the road some way of globally intercepting DB calls to return SQL could replace some of the individual versions?

@jim-parry
Copy link
Contributor

If we are adding two parameters to a method signature, shouldn't we be updating the user guide too?

@MGatner
Copy link
Member

MGatner commented Sep 24, 2019

Yes, updates to user_guide_src/source/database/query_builder.rst. If @michalsn isn't around I could pass these tonight as a separate PR.

@MGatner
Copy link
Member

MGatner commented Sep 24, 2019

Thanks so much @michalsn, looks great.

@MGatner MGatner merged commit 3a0b8df into codeigniter4:develop Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Query Builder getWhere Crash

3 participants